Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORE-2699 Quotas: prepare for using the quota store in client_quota_translator #18777

Conversation

pgellert
Copy link
Contributor

@pgellert pgellert commented Jun 4, 2024

A set of fixup commits and refactors that lead up to the next PR which will start using the client_quota::store inside client_quota_translator. These commits are extracted to aid review.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

  • none

@pgellert pgellert requested a review from a team June 4, 2024 19:47
@pgellert pgellert self-assigned this Jun 4, 2024
@pgellert pgellert requested review from graphcareful and BenPope and removed request for a team June 4, 2024 19:47
src/v/kafka/server/quota_manager.cc Outdated Show resolved Hide resolved
src/v/kafka/server/client_quota_translator.h Outdated Show resolved Hide resolved
pgellert added 6 commits June 5, 2024 14:02
We should start each test with clean configs to prevent them from
interfering with eachother.
A follow up commit adds kafka/types.h as a transitive dependency to the
client_quota_translator_test.cc which causes the client_id constant in
the test file to clash with the client_id type alias from types.h.
This fixes a bug in the quota values being returned for group-based
quota keys where we would incorrectly fall back to the default values
for group-based keys when the correct quota is really "not applicable"
(i.e. `std::nullopt`).

This is not an observable bug because these quota values are never
really accessed. If the request had a group-based applicable quota, it
would both have a group-based key and its limit. It's not possible for a
request to have a group-based key and a default quota value.

We fix this bug now because we are about to integrate with the quota
store, which makes this bug more apparent.
This wires the sharded quota_store into the client_quota_translator.
There's no behaviour change yet (the quota store is not accessed yet),
so this is merely a refactor.
This refactors the quota lookup methods to be generic across the
different request types in a way that allows us to hook in quotas from
the quota store in follow up commits.

Refactor only, no behaviour change.
@pgellert pgellert force-pushed the quotas/integrate-quota-store-translator-basics branch from ab535f3 to c1b933d Compare June 5, 2024 13:27
@pgellert
Copy link
Contributor Author

pgellert commented Jun 5, 2024

Force-pushed to address Ben's feedback and to get the quota_manager_rpbench to compile.

@pgellert pgellert merged commit 6bc4a00 into redpanda-data:dev Jun 5, 2024
18 checks passed
@pgellert pgellert deleted the quotas/integrate-quota-store-translator-basics branch June 5, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants